Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major refactor, API changes, new tests #8

Merged
merged 36 commits into from
Nov 8, 2024
Merged

Major refactor, API changes, new tests #8

merged 36 commits into from
Nov 8, 2024

Conversation

keevindoherty
Copy link
Contributor

Summary

This PR introduces a major project restructure, some API changes, and new tests.

At a high level, the new project structure looks something like this:

mac
├── LICENSE
├── benchmarks /* python scripts for benchmarking */
├── data /* sample datasets */
├── docs  /* doc files, compiled docs */
├── examples  /* example scripts using MAC, including for pose-graph SLAM */
├── mac  /* actual library code; this is what you 'pip install' */
│   ├── optimization
│   ├── solvers
│   └── utils
├── requirements.txt
└── tests. /* test code; mirrors the library structure */
    ├── optimization
    ├── solvers
    └── utils

I won't go into detail about the content that is consistent with what is on main, but in terms of the new files, we have:

  • benchmarks is for Python performance benchmarks. These are not meant to be unit tests, but instead report on solution quality and timing statistics for a few representative datasets.
  • mac/optimization is for the core optimization routines we use. This includes the Frank-Wolfe implementation in frankwolfe.py and implementation of linear program oracles for a variety of compact, convex constraint sets in constraints.py.
  • mac/utils is a directory containing what was formerly in the utils.py and cholesky_utils.py files. These are now split up into separate utility files conversions.py for NetworkX < > MAC format conversions, graphs.py for Laplacian constructors and general graph manipulation utils, rounding.py for rounding to the feasible sets implemented in constraints.py, and cholesky.py for Suitesparse Cholesky support. I also moved fiedler.py containing our custom hooks into the NetworkX TRACEMIN-Fiedler implementation here, but I might move these to a separate fiedler directory along with the specialized Cholesky utilities specific to Fiedler value computation.
  • mac/solvers contains implementations for three solvers: MAC, GreedyESP [Khosoussi et al. 2019] - a greedy algorithm for maximizing the (reduced) Laplacian determinant, and "GreedyEig" which is a greedy algorithm for optimizing algebraic connectivity.

Bugs squashed

  • In frankwolfe.py the relative duality gap convergence criterion sets a threshold of the form (dual_upper_bound - f(x)) / f(x) < relative_duality_gap_threshold. This will fail if f(x) happens to be zero, and will "false positive" if f(x) is negative. We fix this by moving the f(x) in the denominator to the right-hand side and taking the absolute value. So we now compute (upper - f(x)) < relative_duality_gap_threshold * f(x). This is more robust numerically, but of course if f(x) is indeed zero, it will never trigger. I think this is OK for now. The fix would likely be to add some absolute duality gap threshold, but this seems pretty hard to specify, since we do not know the "scale" of f(x).
  • In conversions.py, there were a variety of subtly bugs converting between NetworkX graphs and the MAC edge list format. These were mostly because the code was "overfit" to the specific case where the NetworkX graphs we were interested in were all unweighted (or had weights uniformly equal to one). This is fixed now.

Testing

New tests added to tests/... including:

  • Tests for the core optimization libraries, running Frank-Wolfe on some simple constrained concave maximization problems (e.g. quadratic objective). We now explicitly test cases like f(x) \approx 0 which would have caused a "divide by zero" error before (see above)
  • Tests for utils libraries. We now test for connected / disconnected graphs in our Fiedler value computations. The latter test (disconnected graph) is currently failing. We need to fix this to improve support for features onkjd/no-fixed-graph, but we actually have a test now! We also test our routines for converting between NetworkX and MAC, which actually had some subtle bugs (see above).

@keevindoherty keevindoherty marked this pull request as draft August 4, 2024 20:21
@keevindoherty
Copy link
Contributor Author

I'm temporarily skipping the disconnected graph test for the Fiedler vector / value computation. I think fixing this should be relatively straightforward - just add a custom TRACEMIN solver to better handle this. A naive solution would be to add some regularization to the anchored Laplacian that gets built inside the solver, e.g. as M = L + reg * I, then compute λ₂(L) = λ₂(M) - reg along with a corresponding eigenvector.

If λ₂(L) = 0, then we just have to return any eigenvector in {v | v ∈ ker(L), v ⟂ 1}. (This could occur, for example, if the corresponding graph had more than 2 connected components).

There are interesting potential performance considerations here, I think. It might be better to attempt to solve for the Fiedler value and vector without this regularization, then catch the internal linear system solver exception and fall back to the regularized version? I'll leave this for a future PR, though.

The remaining TODOs as far as I can tell are:

  • Hook the new bits up with the pose graph stuff
  • Implement benchmarks here or make an issue to add in a future PR
  • Update READMEs

@keevindoherty keevindoherty marked this pull request as ready for review November 8, 2024 02:21
@keevindoherty keevindoherty merged commit 079663f into main Nov 8, 2024
1 check passed
@keevindoherty keevindoherty deleted the kjd/refactor branch November 8, 2024 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant